Feature/ex 3 by nikiJava · Pull Request #3 · nikiJava/AndroidAcademyNewsApp

nikiJava · 2018-11-17T09:05:47Z

View Exercise 3.

…different items height.

…esult of branches merging).

colriot

Отревьюил

colriot · 2018-11-30T13:19:06Z

app/build.gradle

 }

 dependencies {
+    implementation fileTree(dir: 'libs', include: ['*.jar'])


Зачем папку опять добавил? Нужны какие-то внешние джарники?

Убрал папку.

colriot · 2018-11-30T13:20:38Z

app/build.gradle


 dependencies {
+    implementation fileTree(dir: 'libs', include: ['*.jar'])
+    implementation "androidx.cardview:cardview:1.0.0"


Саппортные либы должны быть одной версии, для чего имеет вынести её в переменную: для cardview и recyclerview

Вынес в отдельную переменную.

colriot · 2018-11-30T13:33:01Z

...in/java/ru/nikijava/androidacademynewsapp/presentation/news/details/NewsDetailsActivity.java

+        super.onCreate(savedInstanceState);
+        setContentView(R.layout.activity_news_details);
+        news = (News) getIntent().getSerializableExtra(NEWS_KEY);
+        setSupportActionBar(findViewById(R.id.toolbar));


Лучше сделать проверку на null, т.к UI может измениться, а об отсутствии тулбара узнаешь уже по крэшу

Специально не проверял на null, поскольку таким образом можно замаскировать собственную или чужую ошибку (кто-то случайно поменял id у тулбара, или вовсе убрал без предупреждения), а так, по крэшу при тестировании я сразу ошибку увижу, что тулбар почему-то не найден на макете, увижу строчку, в которой ошибка.

Подход имеет смысл. Зависит от перспективы и приоритетов.
Добро 👍 Классно, что оспариваешь комменты и аргументируешь!

colriot · 2018-11-30T13:34:44Z

...in/java/ru/nikijava/androidacademynewsapp/presentation/news/details/NewsDetailsActivity.java

+public class NewsDetailsActivity extends AppCompatActivity {
+
+    private static String NEWS_KEY = "news_key";
+    private final DateFormatter dateFormatter = new DateFormatter();


Даже static можно

Согласен - нет смысла каждый раз создавать новый инстанс объекта при открытии экрана.

colriot · 2018-11-30T13:35:42Z

app/src/main/java/ru/nikijava/androidacademynewsapp/DateFormatter.java

+
+public class DateFormatter {
+
+    public CharSequence formatDateTime(Context context, Date dateTime) {


Вообще похоже на то, что тебе не нужен инстанс. Это просто статический метод, самый обычный.

Если делать метод как static, то в таком случае и вызывать его нужно будет не через инстанс, а через класс. И как явную зависимость этот класс уже не объявишь потом - как такового смысла в этом не будет. Или такие классы с небольшой и просто логикой не стоит всегда указывать, как явную зависимость? Или суть в том, что у этого класса нет состояния и, следовательно, создавать инстанс смысла нет?

Нет состояния, и он больше как Utils-класс. Если бы у тебя была возможность разные конфигурации сделать, подменять реализацию, то да - инстанс

Исправил метод на static

colriot · 2018-11-30T14:47:08Z

app/src/main/res/layout/item_news.xml

+            android:id="@+id/tvBody"
+            android:layout_width="0dp"
+            android:layout_height="wrap_content"
+            android:layout_marginTop="16dp"


Базовые димены хорошо бы вынести в dimens.xml

Константы использовал только для паддингов. Для layout_margin константы не использовал, так как все вьюшки внутри ConstraintLayout располагаются.

colriot · 2018-11-30T14:55:15Z

app/src/main/res/layout/item_news.xml

+        <ImageView
+            android:id="@+id/ivImage"
+            android:layout_width="100dp"
+            android:layout_height="0dp"


Сделал, чтобы оно растягивалось при ресайзе? Но я не вижу констрейнта на левый край вьюшки. Тогда почему так?

Чтобы высота картинки была всегда одинаковой - top вьюшки всегда был на уровне top'a заголовка (tvTitle), а bottom вьюшки был на уровне bottom'a описания новости (tvBody). Чтобы такое поведение обеспечивалось, для заголовка и описания задал фиксированное количество строк (android:lines="2" и android:lines="3" соответственно).

Упустил то, что ширина вью фиксированная. Всё норм с точки зрения реализации, а с точки зрения UX - спорно - картинка будет вертикальная. Размер шрифта может менять, т.е и высотка текста, и высота картинки тоже, а ширина фикси. И ещё: когда будут короткие тексты, у тебя будет помногу пустого места внизу - возможно не есть гут, и данамическая высота была бы лучше.

Сделал с помощью барьеров для картинки соотношение высоты к ширине как 1:1 - позволило избавиться от жёстко указанной ширины. На мой взгляд - это всё же минус, когда в одном и том же списке у одинаковых айтемов высота картинки будет разной, поскольку будет зависеть от размера заголовка и дескрипшена. Возможно, что это уже субъективной вопрос, но опыт пользования различными новостными приложениями да и просто приложениями, где есть список, подсказывает, что картинка всё должна быть одинаковой на всех айтемах. Да и желательно квадратной (1:1).

colriot · 2018-11-30T14:55:59Z

app/src/main/res/layout/item_news_big.xml

+            android:id="@+id/tvTitle"
+            android:layout_width="0dp"
+            android:layout_height="wrap_content"
+            android:background="#b4c6c5c5"


Никогда не хардкодь цвета

Убрал цвет константу.

colriot · 2018-11-30T14:56:15Z

app/src/main/res/layout/item_news_big.xml

+            android:layout_width="wrap_content"
+            android:layout_height="wrap_content"
+            android:layout_marginStart="16dp"
+            android:layout_marginTop="8dp"


Размеры то из констант, то хардкод

Константы использовал только для паддингов. Для layout_margin константы не использовал, так как все вьюшки внутри ConstraintLayout располагаются.

colriot · 2018-11-30T14:57:45Z

delegateadapter/src/main/java/ru/nikijava/adapterdelegate/CompositeDelegateAdapter.java


        public Builder() {
-            typeToAdapterMap = new SparseArray<>();
+            typeToAdapterMap = new HashMap<>();


Лучше юзать ArrayMap, для более эффективной работы по памяти. Т.к тебе не надо постоянно изменять мапу, а только несколько раз добавить в неё, а потом лишь читать.

Заменил на ArrayMap.

…ariable.

…e new instance of it every time.

…memory efficiency

…out.

colriot · 2018-12-06T11:36:10Z

app/src/main/res/values/colors.xml

    <color name="colorPrimaryDark">#ffd4a515</color>
    <color name="colorAccent">#ff962326</color>
    <color name="window_background">?android:attr/colorBackground</color>
+    <color name="background_transparent_grey">#b4c6c5c5</color>


Если ты прям акцент хочет на цвете сделать (и много где его юзать), то ок.
А вот если тут акцент скорее на назначении, типа "цвет фона карточки", то может лучше именно так его и назвать. Т.к он может поменяться, но не надо будет ещё и название менять (если он станет, например, синим). Но это не обязательно к исправлению, просто имей в виду.

Создал отдельную константу для фона заголовка карточки

colriot · 2018-12-06T11:38:55Z

app/src/main/java/ru/nikijava/androidacademynewsapp/DateFormatter.java

+
+public class DateFormatter {
+
+    public CharSequence formatDateTime(Context context, Date dateTime) {


Нет состояния, и он больше как Utils-класс. Если бы у тебя была возможность разные конфигурации сделать, подменять реализацию, то да - инстанс

colriot

В целом approve. Есть мелкие замечания в ответах на комменты, то не критичны

colriot · 2018-12-06T11:45:37Z

app/src/main/res/layout/item_news.xml

+        <ImageView
+            android:id="@+id/ivImage"
+            android:layout_width="100dp"
+            android:layout_height="0dp"


Упустил то, что ширина вью фиксированная. Всё норм с точки зрения реализации, а с точки зрения UX - спорно - картинка будет вертикальная. Размер шрифта может менять, т.е и высотка текста, и высота картинки тоже, а ширина фикси. И ещё: когда будут короткие тексты, у тебя будет помногу пустого места внизу - возможно не есть гут, и данамическая высота была бы лучше.

DateFormatter has no behaviour and so it is most preferably to not create redundant instances.

it helps to save scroll position in time of orientation changing.

nikiJava added 11 commits November 11, 2018 21:14

Implement news section.

944c5da

Add another layout (viewType) for criminal news.

c299774

Add placeholder and error drawable into news list.

df14019

Merge branch 'develop' into feature/ex-3

86afeed

Add CollapsingToolbar on news details screen.

3858912

Reformat news list layout.

3675333

Replace GridLayoutManager with StaggeredGridLayoutManager because of …

7c5bab9

…different items height.

Remove duplicate keys in gradle properties.

cae3a12

Remove redundant chars remained from merge conflict in styles.xml.

bdf3bd1

Remove redundant strings.xml with the only Russian locale (ru-rRu) (r…

6d36e46

…esult of branches merging).

Add contentDescription for images in news items.

8a82737

colriot suggested changes Nov 30, 2018

View reviewed changes

nikiJava added 8 commits December 1, 2018 15:41

Remove redundant kotlin gradle plugin.

ceb6a58

Extract androidx version of RecyclerView and CardView into separate v…

8dd910b

…ariable.

Make DateFormatter field in news details screen static so don't creat…

69c66b3

…e new instance of it every time.

Extract background color from item_news_big into color resources.

2665259

Replace HashMap with ArrayMap in CompositeDelegateAdapter for better …

0f67b6d

…memory efficiency

Remove from build gradle compiling .jar files from /libs.

4523c9b

Remove redundant contentDescription for RecyclerView in news list lay…

554ea4f

…out.

Remove redundant attributes of ConstraintLayout in news details layout.

069cefb

colriot reviewed Dec 6, 2018

View reviewed changes

colriot approved these changes Dec 6, 2018

View reviewed changes

nikiJava added 4 commits December 8, 2018 15:18

Set DateFormatter's formatDateTime method as static

aee79da

DateFormatter has no behaviour and so it is most preferably to not create redundant instances.

Create separate color constant for big news item title background

cdd95e1

Implement ratio 1:1 for image in news item

0cbd83f

Set StaggeredGridLayoutManager for news list in all configurations

c3ff950

it helps to save scroll position in time of orientation changing.


		public class DateFormatter {

		public CharSequence formatDateTime(Context context, Date dateTime) {

Conversation

nikiJava commented Nov 17, 2018

Uh oh!

colriot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colriot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants